Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move global tracer and substitute agent-core types with API types where this is trivially possible. #3054

Merged
merged 17 commits into from
Mar 28, 2023
Merged

Move global tracer and substitute agent-core types with API types where this is trivially possible. #3054

merged 17 commits into from
Mar 28, 2023

Conversation

raphw
Copy link
Contributor

@raphw raphw commented Mar 10, 2023 •
edited
Loading

This change is mainly generated by IDEA code substitution, with minor adjustments. By using API types, the dependency on implementation is reduced and the required API surface becomes more visible. This change is in preparation of removing the core module as a dependency of the internal plugins.

@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels Mar 10, 2023
@github-actions
Copy link

👋 @raphw Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

@raphw
Copy link
Contributor Author

raphw commented Mar 10, 2023 •
edited
Loading

This is mostly auto-generated. I do however wonder about one difference, and that is the change here: main...raphw:apm-agent-java:reduce-api-exposure#diff-732acc761e4310277786f5059749ac54e3f0dcfa17d8976ca3892fa7fb7d5c52L154

This particular use of TraceContext was previously a unique application of this API. Is it the same as using addLink (addSpanLink) to which I normalized this? It leads to the same asChildOf call, but also adds a link entry.

@raphw raphw mentioned this pull request Mar 10, 2023
Open
@apmmachine
Copy link
Contributor

apmmachine commented Mar 10, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-03-24T23:23:53.810+0000

  • Duration: 14 min 13 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

JonasKunz
JonasKunz approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval to allow running on CI, thorough review will follow.

JonasKunz
JonasKunz reviewed Mar 23, 2023
View reviewed changes
CHANGELOG.asciidoc Outdated Show resolved Hide resolved
JonasKunz
JonasKunz reviewed Mar 23, 2023
View reviewed changes
Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments.
In addition I think the tests are currently failing and still need some attention.

...-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ApiInstrumentation.java Outdated Show resolved Hide resolved
...-sdk/apm-aws-sdk-1-plugin/src/main/java/co/elastic/apm/agent/awssdk/v1/helper/SQSHelper.java Show resolved Hide resolved
.../src/main/java/co/elastic/apm/agent/awssdk/common/AbstractDynamoDBInstrumentationHelper.java Show resolved Hide resolved
...ent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/elastic/apm/agent/grpc/GrpcHelper.java Outdated Show resolved Hide resolved
...kafka-base-plugin/src/main/java/co/elastic/apm/agent/kafka/KafkaProducerInstrumentation.java Show resolved Hide resolved
...plugin/src/main/java/co/elastic/apm/agent/kafka/SpringKafkaBatchListenerInstrumentation.java Outdated Show resolved Hide resolved
...-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletApiAdvice.java Outdated Show resolved Hide resolved
raphw and others added 16 commits March 24, 2023 23:54
@raphw
Move global tracer and substitute agent-core types with API types whe…
f0af9e5 
…re this is trivially possible.

This change is mainly generated by IDEA code substitution, with minor adjustments. By using API types, the dependency on implementation is reduced and the required API surface becomes more visible. This change is in preparation of removing the core module as a dependency of the internal plugins.
@raphw
Remove helper method in favor of adding link directly which improves …
edeb850 
…code readability.
@raphw @JonasKunz
Update apm-agent-plugins/apm-kafka-plugin/apm-kafka-spring-plugin/src…
2397813 
…/main/java/co/elastic/apm/agent/kafka/SpringKafkaBatchListenerInstrumentation.java

Co-authored-by: Jonas Kunz <[email protected]>
@raphw @JonasKunz
Update apm-agent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/el…
d531e32 
…astic/apm/agent/grpc/GrpcHelper.java

Co-authored-by: Jonas Kunz <[email protected]>
@raphw
Fix refactoring errors.
eb3cb68
@raphw
Avoid premature unpacking of tracer instance.
79ec971
@raphw
Avoid further tracer castings.
2b09b25
@raphw
Fix URL fill method.
db0cb5b
@raphw
Normalize naming.
4cc52ec
@raphw
Attempt to avoid another premature static initialization.
de4ee00
@raphw
Run configuration update.
59cbb0a
@raphw
Fix test that mocks tracer class.
452490b
@raphw
Fix other tests that require mock tracer.
3199503
@raphw
Another attempt of fixing documentation.
0c8b1c2
@raphw
Outcomment failing lines of JMS test.
37f54f5
@raphw
Fix integration test to reflect new JMS tracing.
d5c9c4d
@raphw raphw force-pushed the reduce-api-exposure branch from ee20b86 to d5c9c4d Compare March 24, 2023 22:55
@raphw
Copy link
Contributor Author

raphw commented Mar 24, 2023

Tests should be fixed by now. Mainly missing mock configurations to reflect require calls properly. And of course JmsInstrumentationIT needed an adjustment with regards to the span link change.

JonasKunz
JonasKunz approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are good to merge!
Side Note for the next PR: Would be great if you could use merge instead of force-push. This allows us to use the "Review changes since last review" feature from GH when doing review iterations.

@raphw
Copy link
Contributor Author

raphw commented Mar 27, 2023

Will do, I rebased to main, in hope that a mysterious test error would disappear by it, that's what the force push was about.

@JonasKunz
Copy link
Contributor

run elasticsearch-ci/docs

@JonasKunz JonasKunz merged commit 1fd41bc into elastic:main Mar 28, 2023
@SylvainJuge SylvainJuge mentioned this pull request Apr 12, 2023
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyalkoren eyalkoren eyalkoren left review comments

@JonasKunz JonasKunz JonasKunz approved these changes

Assignees
No one assigned
Labels
agent-java community Issues and PRs created by the community triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@raphw @apmmachine @JonasKunz @eyalkoren